Skip to content

Better specify expected Node.js version and update to v20.19.2 #99

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Better specify expected Node.js version and update to v20.19.2 #99

wants to merge 1 commit into from

Conversation

doshitan
Copy link
Contributor

@doshitan doshitan commented May 30, 2025 •
edited by github-actions bot
Loading

The container image was pinned to Node.js 20.18.1, see Dockerfile:

FROM node:20.18.1-bullseye-slim AS base

But attempting to use 20.18.1 to build Storybook, you encounter an error:

{
  column: 35,
  file: '/home/doshitan/projects/platform-test-nextjs/app/.storybook/main.mjs',
  length: 11,
  line: 29,
  lineText: '      nextConfigPath: path.resolve(import.meta.dirname, "../next.config.js"),',
  namespace: '',
  suggestion: ''
}
"import.meta" is not available with the "cjs" output format and will be empty
SB_CORE-SERVER_0007 (MainFileEvaluationError): Storybook couldn't evaluate your .storybook/main.mjs file.

This is due to a recent change[1] that switched to using import.meta.dirname,
a feature of Node.js ESM loading, instead of the legacy __dirname. But
automatic detection and loading a file as ESM is behind a feature flag until
Node.js v20.19.0[2]. Thus breaking the Storybook build for the project's
"official" Node 20.18.1 version and container workflow.

So:

  • Update Node.js version to 20.19.2
  • Add specific version requirement to package.json, which at least will
    display a warning to folks installing packages outside the container
    environment.
  • Use package.json specification to load the correct Node.js version in CI

Which should help keep all the different environments in sync. And the new
Node.js module behavior is the default in later versions as well, so might as
well lean into it.

[1] navapbc/template-application-nextjs#392
[2] https://nodejs.org/en/blog/release/v20.19.0/#requireesm-is-now-enabled-by-default

Testing

Node.js 20.18.1

❯ npm run storybook-build

> app@0.1.0 storybook-build
> storybook build

@storybook/core v8.4.7

info => Cleaning outputDir: storybook-static
{
  column: 35,
  file: '/home/doshitan/projects/platform-test-nextjs/app/.storybook/main.mjs',
  length: 11,
  line: 29,
  lineText: '      nextConfigPath: path.resolve(import.meta.dirname, "../next.config.js"),',
  namespace: '',
  suggestion: ''
}
"import.meta" is not available with the "cjs" output format and will be empty
SB_CORE-SERVER_0007 (MainFileEvaluationError): Storybook couldn't evaluate your .storybook/main.mjs file.

Original error:
TypeError [ERR_INVALID_ARG_TYPE]: The "paths[0]" argument must be of type string. Received undefined
    at Object.resolve (node:path:1169:7)
    at Object.<anonymous> (/.storybook/main.mjs:29:28)
    at Module._compile (node:internal/modules/cjs/loader:1469:14)
    at Module._compile (/node_modules/esbuild-register/dist/node.js:2258:26)
    at Module._extensions..js (node:internal/modules/cjs/loader:1548:10)
    at Object.newLoader [as .mjs] (/node_modules/esbuild-register/dist/node.js:2262:9)
    at Module.load (node:internal/modules/cjs/loader:1288:32)
    at Module._load (node:internal/modules/cjs/loader:1104:12)
    at Module.require (node:internal/modules/cjs/loader:1311:19)
    at require (node:internal/modules/helpers:179:18)
    at loadMainConfig (./node_modules/@storybook/core/dist/common/index.cjs:17511:11)
    at async buildStaticStandalone (./node_modules/@storybook/core/dist/core-server/index.cjs:35381:11)
    at async withTelemetry (./node_modules/@storybook/core/dist/core-server/index.cjs:35757:12)
    at async build (./node_modules/@storybook/core/dist/cli/bin/index.cjs:2555:3)
    at async s.<anonymous> (./node_modules/@storybook/core/dist/cli/bin/index.cjs:2661:7)
{
  column: 35,
  file: '/home/doshitan/projects/platform-test-nextjs/app/.storybook/main.mjs',
  length: 11,
  line: 29,
  lineText: '      nextConfigPath: path.resolve(import.meta.dirname, "../next.config.js"),',
  namespace: '',
  suggestion: ''
}
"import.meta" is not available with the "cjs" output format and will be empty
{
  column: 35,
  file: '/home/doshitan/projects/platform-test-nextjs/app/.storybook/main.mjs',
  length: 11,
  line: 29,
  lineText: '      nextConfigPath: path.resolve(import.meta.dirname, "../next.config.js"),',
  namespace: '',
  suggestion: ''
}
"import.meta" is not available with the "cjs" output format and will be empty

Node >= 20.19.0

❯ npm run storybook-build

> app@0.1.0 storybook-build
> storybook build

@storybook/core v8.4.7

info => Cleaning outputDir: storybook-static
info => Loading presets
info => Building manager..
info => Manager built (183 ms)
info => Building preview..
info Addon-docs: using MDX3
info => Using implicit CSS loaders
info => Using SWC as compiler
info => Using default Webpack5 setup
10% building 0/1 entries 0/0 dependencies 0/0 modulesinfo => Copying static files: public at storybook-static
10% building 0/1 entries 1/1 dependencies 0/1 modulesinfo Using tsconfig paths for react-docgen

[...lots of deprecation warnings...]

info => Preview built (52 s)
info => Output directory: /home/doshitan/projects/platform-test-nextjs/app/storybook-static

The warning if running a Node.js version that doesn't match package.json requirements (in this example 20.19.0 instead of 20.19.2):

❯ npm i
npm warn EBADENGINE Unsupported engine {
npm warn EBADENGINE   package: 'app@0.1.0',
npm warn EBADENGINE   required: { node: '20.19.2', npm: '>=10.0.0' },
npm warn EBADENGINE   current: { node: 'v20.19.0', npm: '10.8.2' }
npm warn EBADENGINE }

Preview environment for app

♻️ Environment destroyed ♻️

@github-actions GitHub Actions
Copy link

Coverage report for app

St.❔
 Category Percentage Covered / Total
🟢 Statements 97.92% 47/48
🟢 Branches 85.71% 6/7
🟢 Functions 100% 8/8
🟢 Lines 100% 40/40

Test suite run success

5 tests passing in 3 suites.

Report generated by 🧪jest coverage report action from 24deaf7

@doshitan doshitan changed the title Better specify expected Node.js version Better specify expected Node.js version and update to v20.19.2 May 30, 2025
@doshitan doshitan mentioned this pull request May 30, 2025
Merged
lorenyu
lorenyu approved these changes May 30, 2025
View reviewed changes
Copy link
Contributor

@lorenyu lorenyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM appreciate the attention to detail. Is there any value in encapsulating the setup node step in a custom action? That way if we want to change anything such as the setup-node@v4 to @v5 we can do it one place

@doshitan
Copy link
Contributor Author

Merged in navapbc/template-application-nextjs#399

@doshitan doshitan closed this May 30, 2025
@doshitan
Copy link
Contributor Author

Is there any value in encapsulating the setup node step in a custom action? That way if we want to change anything such as the setup-node@v4 to @v5 we can do it one place

Hmm, possibly. There's not many spots using it today and for just version swaps, sed s// is easy enough, so doesn't feel super valuable to at the moment. Though the cache params push it to almost there.

@lorenyu
Copy link
Contributor

lorenyu commented May 30, 2025

@doshitan agree, it's kind of on the fence for me too, fine to keep as is for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lorenyu lorenyu lorenyu approved these changes

@mqwebster-nava mqwebster-nava Awaiting requested review from mqwebster-nava

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@doshitan @lorenyu